Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Pass cancellation token to Dequeue method, and rewrite comment #51794

Merged
merged 1 commit into from
Mar 11, 2021
Merged

Pass cancellation token to Dequeue method, and rewrite comment #51794

merged 1 commit into from
Mar 11, 2021

Conversation

davidwengier
Copy link
Contributor

If our server got shut down by the client, and we happened to be waiting to dequeue the next item from the queue, we would log it as an error because it came from an unknown cancellation token. This correctly links everything up.

Then I updated the comment because who understands Past Dave™

@davidwengier davidwengier requested a review from a team as a code owner March 11, 2021 02:39
sharwell
sharwell approved these changes Mar 11, 2021
View reviewed changes
Copy link
Member

@sharwell sharwell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm assuming we are sure DequeueAsync processes the _cancelSource cancellation before whatever other cancellation was involved to lead to the log.

sharwell
sharwell reviewed Mar 11, 2021
View reviewed changes
src/Features/LanguageServer/Protocol/Handler/RequestExecutionQueue.cs
@@ -230,8 +230,10 @@ private async Task ProcessQueueAsync()
}
catch (OperationCanceledException e) when (e.CancellationToken == _cancelSource.Token)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 The other way to do this is:

Suggested change
catch (OperationCanceledException e) when (e.CancellationToken == _cancelSource.Token)
catch (OperationCanceledException) when (_cancelSource.IsCancellationRequested)

@davidwengier
Copy link
Contributor Author

I'm assuming we are sure DequeueAsync processes the _cancelSource cancellation before whatever other cancellation was involved to lead to the log.

I'm not sure exactly what you mean, but there is no "other" cancellation. The log happened because of the call to _queue.Complete() cancels the pending DequeueAsync call, but without a cancellation token in the exception. Or at least not the one we were checking for.

@sharwell
Copy link
Member

sharwell commented Mar 11, 2021 •
edited
Loading

I'm not sure exactly what you mean, but there is no "other" cancellation.

The call to _queue.Complete() calls TrySetCanceled() on any remaining DequeueAsync waiters. It's important to ensure that all of those waiters have called TrySetCanceled(token) prior to the call to Complete() so the right cancellation token makes it all the way through to the location where you checked it.

Looking at the code, I believe this is correctly handled right now.

@davidwengier davidwengier merged commit 27913e0 into dotnet:main Mar 11, 2021
@davidwengier davidwengier deleted the DontReportCancellations branch March 11, 2021 22:24
@allisonchou allisonchou added this to the Next milestone Mar 12, 2021
@allisonchou allisonchou modified the milestones: Next, 16.10.P2 Mar 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sharwell sharwell sharwell approved these changes

@dibarbet dibarbet Awaiting requested review from dibarbet

Assignees
No one assigned
Labels
Area-IDE
Projects
None yet
Milestone
16.10.P2
Development

Successfully merging this pull request may close these issues.
3 participants
@davidwengier @sharwell @allisonchou